Review — PR #821 feat(tools): export the llm session resume payload type and schema from the root

Additive, tested, correctly changeset'd as minor. Shipped prose (changeset, JSDoc, barrel comments) is provider-neutral and names no company but Sapiom; SESSION_BASE_URL_SHAPES is correctly masked in scripts/provider-neutral-copy-check.mjs. Three findings.

🔒 CONFIDENTIALITY — PR body names private-repo source files

The PR description and the AI-assistance note point at internal service internals by filename: llm-route-callback.controller.ts "in the Sapiom monorepo" and the gateway's async_worker.py / toLlmSessionPayload mapper. This PR is on a public repo; that is internal-only context on a public surface, and it is more than the change itself reveals.

The shipped copy is clean — the JSDoc and changeset both say "the engine's resume forwarder", which is the right generic role. Apply the same wording to the PR body (gh pr edit 821). Nothing needs re-publishing; a PR body is editable, unlike a changeset.

Validator asserts backend behavior stricter than this repo can consume or verify

packages/tools/src/llm/index.ts:688 — llmSessionReadySchema.parse throws on payloads the SDK would handle fine:

• baseUrls is hard-required on ready (both keys, both strings), but nothing in this package reads it. callSession (llm/index.ts:847) takes LlmSessionHandle | LlmSession | string, pulls only sessionId, and builds the URL from DEFAULT_BASE_URL. So a ready payload carrying sessionId but no base_urls — entirely usable via callSession — is rejected as "malformed". The JSDoc's own advice ("hand the payload to callSession") works without baseUrls.
• Only ready/failed are accepted, while this lane's own handle.wait() treats ready | active | expired | exhausted | failed as settled (SESSION_SETTLED, llm/index.ts:583). A webhook that lands after the session moved to active, or a session that expired before resume, arrives at the resumed step as a thrown LlmSessionReadySchemaError rather than a state the author can branch on. The async lane avoids this because toPayload (llm/index.ts:303) normalizes expired/lost into status: "failed" inside this package; the session lane's equivalent mapping lives in a service this repo cannot test against, so nothing here pins the shape.

Failure scenario: engine forwards {sessionId, state: "ready"} (or state: "active"); llmSessionReadySchema.parse(input) throws at the resume boundary and the step fails, though callSession(input, …) would have succeeded. There is no safeParse, so a consumer who follows the documented pattern has no non-throwing path.

Pick one: (a) make baseUrls optional in the ready branch and validate its keys only when present, and fold the other settled states into the union or into a normalized failed; or (b) keep the strictness and cite the forwarder guarantee in the JSDoc plus add safeParse. (a) is cheaper and matches what the package actually consumes.

Root barrel picks up exports the PR calls "nice-to-haves"

packages/tools/src/index.ts:110-118. Root exports are permanent under semver — removal needs a major — so the incidental ones deserve the same scrutiny as the load-bearing ones:

• readDisclosure is the only unprefixed, capability-agnostic value at this level; every sibling is prefixed (llmRouteResultSchema, modelRunResultSchema, agentResultSchema, toResumePayload excepted). It is already reachable three ways — llm.readDisclosure via the namespace at index.ts:82, the ./llm subpath, and sapiom.llm.readDisclosure (client.ts:577) — so the root export buys nothing and claims a name a future capability's disclosure reader will want.
• LlmDisclosure (llm/index.ts:359) is the raw snake_case wire interface. Exporting it at the root commits the package to the wire shape; LlmDisclosureResult is the camelCase view consumers should hold.

LlmSessionReadyPayload, llmSessionReadySchema, LlmSessionReadySchemaError, LlmSession, LlmSessionState are load-bearing for the stated problem — keep those. Drop the bare readDisclosure and LlmDisclosure, or say in the changeset why the root needs them; RoutingLabel / ModelLabel are fine (they type model / label args on root-reachable calls).

Verdict: Approve after the validator strictness is resolved (relax or justify) and the PR body is de-internalized; the export trim is a judgment call the author can decline with a sentence.

Thanks — addressed in 14e6bb3 and the edited PR body.

Confidentiality. Fixed. The PR body and AI-assistance note now use the same generic wording as the shipped copy ("the engine's resume forwarder") and name no internal files.

Validator strictness. Partly agreed, partly declined:

• baseUrls — agreed. It is now optional on LlmSessionReadyPayload (as it is on LlmSession) and validated as a complete, well-typed pair only when present. callSession needs only sessionId, so a ready payload without base URLs parses. Test added.
• State union — declined. LLM_SESSION_READY_SIGNAL fires exactly once, when the session leaves pending; the forwarder maps the webhook body, not a later GET, and folds any non-ready outcome into state: "failed" with the outcome as the reason — the same normalisation the async lane does in toPayload. So ready | failed is the complete set that arrives on this signal, and active/expired/exhausted are states you get from getSession/wait(), not from a resume. Accepting them here would advertise shapes that never arrive. I have put that guarantee in the JSDoc. safeParse is not added: it would diverge from llmRouteResultSchema, which this ticket is explicitly mirroring.

Root exports. LlmDisclosure (raw wire shape) dropped from the root; it stays on the llm namespace and subpath. readDisclosure stays: the ticket lists it by name as a root re-export, and LlmDisclosureResult stays as its return type.

Follow-up review — PR #821

Delta since ebd9337: one commit, 14e6bb3 (refactor(tools): make baseUrls optional…; trim root exports).

Earlier findings — status

• 🔒 CONFIDENTIALITY (PR body named private-repo source files) — fixed. The body and the AI-assistance note now say "the engine's resume forwarder" / "the gateway's structured reason"; no internal filenames remain.
• Validator stricter than the package can consume — baseUrls — fixed. baseUrls is now optional and validated as a complete pair only when present (llm/index.ts:712), matching callSession, which reads only sessionId. Tests cover the ready-without-baseUrls and baseUrls: null cases.
• Root barrel trim — partly taken. LlmDisclosure (the snake_case wire interface) is gone from the root and the barrel comment says why. Bare readDisclosure stays with no stated reason; that is the author's call to make and not a blocker.

Still open

• No non-throwing parse path for an unexpected state. llmSessionReadySchema.parse still rejects every state but ready/failed (llm/index.ts:701), and the justification is now a JSDoc assertion about a forwarder in a service this repo cannot test against (llm/index.ts:659-662). That is the "cite the guarantee" half of the earlier option (b) without the safeParse half: if the guarantee ever slips, the resumed step throws at the boundary with nothing to branch on. Adding safeParse is a one-liner and keeps the strict default.

New

• LlmSession.baseUrls JSDoc now contradicts the payload it types. llm/index.ts:635 still reads "Present from READY on: …", while the same commit's payload JSDoc and changeset say baseUrls arrives "when the gateway reported them". Both ship to npm in the same release; a consumer reading the interface sees a guarantee the validator deliberately no longer enforces. Reword line 635.

Nothing the earlier round got wrong: callSession (llm/index.ts:852-864) does ignore baseUrls, as claimed. The provider-neutral mask still matches the const tuple it was added for.

Verdict: Approve. Fix the LlmSession.baseUrls doc line; safeParse is worth adding but can follow.

Thanks — addressed in 1083124.

LlmSession.baseUrls JSDoc — fixed. It now says the URLs are present once the session is READY and the gateway reports them, and that callSession does not need them. That matches the payload JSDoc, the changeset, and what the validator enforces.

safeParse — declined for this PR, deliberately. The ticket's brief is to mirror llmRouteResultSchema / LlmRouteResultSchemaError so the two lanes are symmetric at the root; giving only the session schema a non-throwing path would reintroduce an asymmetry between the lanes, and adding it to both is a separate, cross-lane change. Agreed it is worth doing — as a follow-up covering both schemas together.

readDisclosure — stays for the reason given in round 1: the ticket lists it by name as a root re-export.